I tested the three demos yarn groupsig-demo, yarn prototype-demo and test_private_secure_variable_groupsig.js and all of them went successful. I tested on Windows + Git Bash.

Benchmark Results (private secure variable groupsig, group size = 4, ~252K constraints):

Witness: 8149.38 ms
Proof: 7233.18 ms
Verify: 528.74 ms
Total: 15952.06 ms

Verified: YES ✅

Setup notes: The build_private_groupsig_circuits.sh script calls snarkjs directly instead of npx snarkjs which fails if snarkjs is not globally installed. The other build scripts use npx snarkjs so it would be better to use it as so in the mentioned script. Additionally, circuit compilation gives many T3001 warnings, but I believe these are inherited from the original repo and they are harmless (confused me at first but not something for us)

Review Findings

1. Hash consistency check in private_secure_variable_groupsig.circom provides no cryptographic guarantee, it checks privHashHi and privHashLo matches their public counterparts. This means there is no connection between proof and any real message, this is a good opportunity for future work I believe
2. The test test_private_secure_variable_groupsig constructs matching inputs, and there is no test for adversarial scenarios. The test is great to see how everything works when things are right, but can be extended for security analysis.
3. No address uniqueness check in circuits, entering same address for multiple member slots still procudes a valid proof, i tested this with groupsig-demo using the same address for both addr1 and addr2 and verification passed, would be better to address this.
4. There are some additional minor/mid issues, but I believe they are out of the scope of this PR

All in all, these are really great additions that serve our purpose well, and thanks for the great work! The README also is very thorough and the performance benchmarks are really nice to have.

Review Findings

1. Hash consistency check in private_secure_variable_groupsig.circom provides no cryptographic guarantee, it checks privHashHi and privHashLo matches their public counterparts. This means there is no connection between proof and any real message, this is a good opportunity for future work I believe

The public counterparts are created by the DIDMultisigController smart contract in on-chain-ssi repo of ASCS during the on-chain verification of a proof generated from private_secure_variable_groupsig.circom. Since the public counterparts are therefore created in a trustworthy way private_secure_variable_groupsig.circom can be minimized in its logic and thus increase performance.

2. The test test_private_secure_variable_groupsig constructs matching inputs, and there is no test for adversarial scenarios. The test is great to see how everything works when things are right, but can be extended for security analysis.

Yes, test_private_secure_variable_groupsig functions as a proof of concept demonstration by testing the desried workflow. An extensive test script is currently missing.

The public counterparts are created by the DIDMultisigController smart contract in on-chain-ssi repo of ASCS during the on-chain verification of a proof generated from private_secure_variable_groupsig.circom. Since the public counterparts are therefore created in a trustworthy way private_secure_variable_groupsig.circom can be minimized in its logic and thus increase performance.

I see, I wasn't thorough with the on-chain-ssi repo yet, probably haven't seen it, thanks for pointing that out!

3. No address uniqueness check in circuits, entering same address for multiple member slots still procudes a valid proof, i tested this with groupsig-demo using the same address for both addr1 and addr2 and verification passed, would be better to address this.

groupsig-demo is the demo script of the original repository and was not done by us as part of this fork. The original repository has many security issues that this fork attempts to address.